Skip to content

Fix KeyError in s3fs _prune_deleted_files function #68452

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 4 commits into
base: 3007.x
Choose a base branch
from
Open

Fix KeyError in s3fs _prune_deleted_files function #68452

wants to merge 4 commits into from
+123 −1

Conversation

@GRomR1
Copy link

@GRomR1 GRomR1 commented Nov 10, 2025 •
edited
Loading

Fixes issue #68335 where fileserver.update fails with KeyError: 'Key' when using S3 buckets in multiple environments per bucket mode.

Problem

The issue occurs in the _prune_deleted_files function where the code assumes a different data structure than what is actually provided. When using multiple environments per bucket mode, the metadata structure is nested differently than expected.

Solution

The fix properly handles the nested bucket structure by iterating through buckets and objects within each bucket to extract the 'Key' field correctly.

Changes Made

1. Core Fix

  • Modified _prune_deleted_files function in salt/fileserver/s3fs.py
  • Fixed KeyError when processing metadata in multiple environments per bucket mode
  • Added proper iteration through bucket structure to handle nested metadata

2. Code Improvements

  • Applied reviewer feedback from @bdrx312 to make code more Pythonic
  • Changed from meta.keys() to meta.values() for cleaner iteration
  • Improved code readability while maintaining functionality

3. Test Coverage

  • Added comprehensive tests for both s3fs configuration modes:
    • test_prune_deleted_files_multiple_envs_per_bucket - Tests the fix for multiple environments per bucket mode
    • test_prune_deleted_files_single_env_per_bucket - Tests single environment per bucket mode to prevent regression
  • Tests verify:
    • Files are properly cached initially
    • Deleted files are removed from cache when calling _prune_deleted_files
    • Remaining files continue to exist in cache
    • The nested metadata structure is handled correctly in multiple env mode

Testing

This fix resolves the KeyError that was preventing fileserver.update from working with S3 buckets in multiple environments per bucket mode. The added tests ensure the fix works correctly and prevent future regressions.

Related Issues

Fixes #68335

Signed-off-by: GRomR1 [email protected]

@GRomR1 GRomR1 requested a review from a team as a code owner November 10, 2025 14:52
@welcome
Copy link

welcome bot commented Nov 10, 2025

Hi there! Welcome to the Salt Community! Thank you for making your first contribution. We have a lengthy process for issues and PRs. Someone from the Core Team will follow up as soon as possible. In the meantime, here's some information that may help as you continue your Salt journey.
Please be sure to review our Code of Conduct. Also, check out some of our community resources including:

There are lots of ways to get involved in our community. Every month, there are around a dozen opportunities to meet with other contributors and the Salt Core team and collaborate in real time. The best way to keep track is by subscribing to the Salt Community Events Calendar.
If you have additional questions, email us at [email protected]. We're glad you've joined our community and look forward to doing awesome things with you!

bdrx312
bdrx312 reviewed Nov 10, 2025
View reviewed changes
twangboy
twangboy requested changes Nov 10, 2025
View reviewed changes
Copy link
Contributor

@twangboy twangboy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please write a test for this as well

@GRomR1
Copy link
Author

GRomR1 commented Nov 17, 2025 •
edited
Loading

twangboy Please write a test for this as well

Thank you for the feedback! I've added comprehensive tests for the s3fs _prune_deleted_files function fix.

The tests cover both configuration modes:

  1. test_prune_deleted_files_multiple_envs_per_bucket - Tests the fix for multiple environments per bucket mode (the original issue [Bug]: Salt fileserver update does not work with S3 buckets #68335)
  2. test_prune_deleted_files_single_env_per_bucket - Tests single environment per bucket mode to ensure no regression

Both tests verify that:

  • Files are properly cached initially
  • Deleted files are removed from cache when calling _prune_deleted_files
  • Remaining files continue to exist in cache
  • The nested metadata structure is handled correctly in multiple env mode

The tests are located in tests/pytests/unit/fileserver/test_s3fs.py and use the existing test infrastructure with moto for S3 mocking.

Please review the tests and let me know if any additional test coverage is needed.

@twangboy twangboy added the test:full Run the full test suite label Nov 24, 2025
@twangboy twangboy added this to the Chlorine v3007.10 milestone Nov 24, 2025
@twangboy twangboy linked an issue Nov 24, 2025 that may be closed by this pull request
[Bug]: Salt fileserver update does not work with S3 buckets #68335
Open
@GRomR1 @twangboy
Fix KeyError in s3fs _prune_deleted_files function
e8ee0f6 
Fixes issue saltstack#68335 where fileserver.update fails with KeyError: 'Key'
when using S3 buckets in multiple environments per bucket mode.

The issue occurs in _prune_deleted_files function where the code assumes
a different data structure than what is actually provided. The fix properly
handles the nested bucket structure by iterating through buckets and objects
within each bucket to extract the 'Key' field correctly.

Signed-off-by: GRomR1 <[email protected]>
@GRomR1 @twangboy
Add changelog entry for issue saltstack#68335 fix
59ce859 
Signed-off-by: GRomR1 <[email protected]>
@GRomR1 @twangboy
Improve code readability in s3fs _prune_deleted_files function
d3f6a07 
Use .values() instead of .keys() for better Pythonic code style.
This addresses the review comment from bdrx312.

Signed-off-by: GRomR1 <[email protected]>
@GRomR1 @twangboy
Add tests for s3fs _prune_deleted_files function
1644dfc 
Add comprehensive tests for both single environment per bucket and
multiple environments per bucket modes to ensure the KeyError fix works
correctly in both scenarios.

Tests cover:
- File deletion from cache when files are removed from S3
- Proper handling of nested metadata structure in multiple env mode
- Cache cleanup for both bucket configuration modes

Signed-off-by: GRomR1 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@twangboy twangboy twangboy approved these changes

+1 more reviewer

@bdrx312 bdrx312 bdrx312 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

test:full Run the full test suite

Projects

None yet

Milestone

Chlorine v3007.10

Development

Successfully merging this pull request may close these issues.

[Bug]: Salt fileserver update does not work with S3 buckets

3 participants

@GRomR1 @bdrx312 @twangboy